DepositToAny slice for the destination #2469

Wunka · 2026-01-07T22:48:22Z

Splitted of from #2324
First of two Payloads that need change to accept slices as destinations for the split of the hotbar to work.

Even if the split of the hotbar is not accepted this could still be necessary. See comment: #2324 (review)

I don't like the allocation solution so if anybody has a better solution it would be very welcome.

The renaming to destinations is to use dest in the for loops and to show that this is different to the other payloads dest parameter

src/Inventory.zig

Wunka · 2026-01-08T10:23:53Z

I changed the priority system between the different destinations to align with #2324 (review)
For testing purposes you can just use #2324 as I have merged this branch into there. What I used for testing is a chest and I just shift-clicked moved items between the chest and my hotbar/inventory

IntegratedQuantum

I immediately see problems with the allocation strategy, please fix them before I review the rest of the code.

IntegratedQuantum · 2026-01-08T21:16:45Z

src/Inventory.zig

-			if(self.dest.type == .creative) return;
-			if(self.dest.type == .crafting) return;
-			if(self.dest.type == .workbench) return;
+			defer if(self.owned) main.stackAllocator.free(self.destinations);


command may run multiple times on the client side, freeing here causes a double free. Instead you can free this in finalize.
Also the stackAllocator is a poor choice, only use the stackAllocator for local allocations, this allocation persists for multiple threads and likely will be ran from a different thread as well.

Also the allocator shouldn't be hardcoded here, instead the caller should pass it.

Lastly who even owns the memory in the other case? How do you keep track of something that gets used possible many frames after passing it?

command may run multiple times on the client side, freeing here causes a double free. Instead you can free this in finalize. Also the stackAllocator is a poor choice, only use the stackAllocator for local allocations, this allocation persists for multiple threads and likely will be ran from a different thread as well.

done changed to the globalAllocator which is used for other payloads

Also the allocator shouldn't be hardcoded here, instead the caller should pass it.

so should I change the finalize to parse over the globalAllocator? That looks better to me at least.

Lastly who even owns the memory in the other case? How do you keep track of something that gets used possible many frames after passing it?

Yeah you are right there... I will look into how I want to solve that. I could maybe just dupe the slice so that its always owned? But I don't want to that in gui.zig where depositToAny is called... I will sleep over it and look tomorrow at the code again.

so should I change the finalize to parse over the globalAllocator? That looks better to me at least.

No, it should be passed on creation, since the finalize call is already really disconnected from the creation.
This would prevent accidentally using mismatched allocators.

switched the owned variable with an allocator. Also did the other issue with an dupe so it should now always be owned.

You could also consider making a separate .init function that allocates everything inside. Then I would be fine with not having the allocator stored since it's all used locally.

IntegratedQuantum · 2026-01-09T21:36:43Z

src/Inventory.zig

-			if(self.dest.type == .creative) return;
-			if(self.dest.type == .crafting) return;
-			if(self.dest.type == .workbench) return;
+			if(self.destinations.len == 0) return;


When would this even happen?

A modified Client can send anything. I just want to put a check in before it later causes bugs.

If this is about safety of client data, then you should put this check in the deserialization function.

Client data should generally be sanitized during or immediately after receiving.

src/Inventory.zig

IntegratedQuantum · 2026-01-09T21:43:01Z

src/Inventory.zig

-			for(self.dest._items, 0..) |*destStack, destSlot| {
-				if(destStack.item == .null and selectedEmptySlot == null) {
-					selectedEmptySlot = @intCast(destSlot);
+			var selectedEmptyInv: u8 = 0;


Why limit this to a u8? It should be usize, there is nothing gained by using a u8, other than future bugs from the intCast.

IntegratedQuantum · 2026-01-09T21:49:03Z

src/Inventory.zig


 		fn serialize(self: DepositToAny, writer: *utils.BinaryWriter) void {
-			writer.writeEnum(InventoryId, self.dest.id);
+			writer.writeInt(u8, @intCast(self.destinations.len));


Use a varint and send a usize
Putting an arbitrary limit on the number and crashing when exceeded is not helpful.
If you want to limit this then you should have a good reason for it, and 256 might definitely be in reach for a deposit to nearby chests function.

src/Inventory.zig

src/gui/gui.zig

IntegratedQuantum · 2026-01-09T21:55:15Z

src/Inventory.zig

+				if(emptySlot != null and (selectedEmptySlot == null or (hasItem and !selectedEmptyInvHasItem))) {
+					selectedEmptySlot = emptySlot;
+					selectedEmptyInv = @intCast(destInv);
+					selectedEmptyInvHasItem = hasItem;


I think this is different from the behavior suggested in #2324 (review), here you seem to be first topping off partially filled stacks in the other inventories.

IntegratedQuantum

Some small things, otherwise it seems reasonable

src/Inventory.zig

Wunka · 2026-01-10T22:52:23Z

rest of the requested changes are also done

Wunka added 3 commits January 7, 2026 23:32

DepositToAny now accepts a slice of destinations

739e2e6

switch to array alloc instead of list

add8af4

format

fbd21a0

Wunka changed the title ~~Deposit slice~~ DepositToAny slice for the destination Jan 7, 2026

Argmaster reviewed Jan 8, 2026

View reviewed changes

src/Inventory.zig Outdated Show resolved Hide resolved

Argmaster reviewed Jan 8, 2026

View reviewed changes

src/Inventory.zig Outdated Show resolved Hide resolved

Argmaster added this to PRs to review Jan 8, 2026

Argmaster moved this to Low Priority in PRs to review Jan 8, 2026

Wunka added 2 commits January 8, 2026 09:18

change from allocated to owned

c8204c6

change priority system

b3a1dee

IntegratedQuantum requested changes Jan 8, 2026

View reviewed changes

IntegratedQuantum moved this from Low Priority to In review in PRs to review Jan 8, 2026

Wunka added 2 commits January 8, 2026 22:57

finalize

f15167a

destinations is always allocated

dfb73e6

IntegratedQuantum requested changes Jan 9, 2026

View reviewed changes

Wunka added 2 commits January 9, 2026 23:32

init, usize and inline

ea634f9

forgot to remove

0787846

IntegratedQuantum requested changes Jan 10, 2026

View reviewed changes

src/Inventory.zig Show resolved Hide resolved

src/Inventory.zig Show resolved Hide resolved

src/Inventory.zig Outdated Show resolved Hide resolved

src/Inventory.zig Outdated Show resolved Hide resolved

small changes

b4a7023

IntegratedQuantum approved these changes Jan 11, 2026

View reviewed changes

IntegratedQuantum merged commit a4263eb into PixelGuys:master Jan 11, 2026
1 check passed

Wunka deleted the DepositSlice branch January 11, 2026 10:04

Wunka mentioned this pull request Jan 11, 2026

tryCrafting now accepts slice #2483

Open

DepositToAny slice for the destination #2469

DepositToAny slice for the destination #2469

Uh oh!

Conversation

Wunka commented Jan 7, 2026

Uh oh!

Uh oh!

Uh oh!

Wunka commented Jan 8, 2026

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wunka commented Jan 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants